שחזור מגיבוי, ונראות למנגנון הגיבוי - #3
Open
amirbiron wants to merge 2 commits into
Open
Conversation
הפער: היה גיבוי ולא הייתה דרך לשחזר ממנו. ארכיון שאפשר לקרוא אינו גיבוי — הוא ארכיון. scripts/restore.py סוגר את זה, ובדיקה שסוגרת את המעגל (יצירה → גיבוי → מחיקת הכול → שחזור → השוואה) מצאה מיד שני באגים אמיתיים, ושניהם היו שקטים: הפרויקט נוצר מחדש עם slug שנגזר מהשם, והמסמכים עם slug שנגזר מהכותרת — במקום אלה שנשמרו בגיבוי. השחזור היה "מצליח", והתוכן היה חוזר תחת כתובות אחרות: כל קישור שנשלח למישהו נשבר. ה-API כבר קיבל slug מפורש ביצירה, והסקריפט פשוט לא שלח אותו. אומת מקצה לקצה מול שרת חי — slug, visibility, שם, קישורים ותוכן חוזרים זהים בדיוק. שלושה מצבי כתיבה, מהבטוח למסוכן: skip (ברירת מחדל, כותב רק מה שלא קיים), upsert (דורס את מה שבגיבוי ומשאיר מה שנוצר אחריו), replace (מוחק ובונה מחדש). לפני כל כתיבה מוצג מה עומד לקרות, והאישור נשאל עם ברירת מחדל N — Enter בטעות לא משחזר כלום. יש --dry-run, ויש מסלול --decrypt לקובץ שמגיע מטלגרם. הסקריפט הפך לאסינכרוני כדי שאותה פונקציית restore תרוץ גם מה-CLI וגם מהבדיקות מול ה-ASGI client. הגרסה הראשונה של הבדיקה המירה את הקוד הסינכרוני לאסינכרוני עם exec על המקור — פתרון שנשבר בשקט בכל שינוי מבנה. נראות: GET /api/backup מחזיר האם המנגנון פעיל, מתי רץ לאחרונה ומתי הבא, ואת רשימת הקבצים; POST /api/backup/run מריץ ידנית. בממשק יש פאנל שמציג את השלוש ולחצן "גיבוי עכשיו". גיבוי שאף אחד לא רואה הוא גיבוי שאף אחד לא מגלה שהפסיק לעבוד, וה-log של Render אינו מקום שבודקים בו כל יום. הנעילה נבדקת עם is_running() לפני התפיסה ולא במקומה: async with לבדו היה גורם לבקשה שנייה להמתין לסיום ואז להריץ גיבוי נוסף. עכשיו היא חוזרת מיד עם 409 — תפוס אינו שבור, ולמשתמש מגיע לדעת מה מהשניים. באג נוסף שהתגלה בדרך: החותמת בשם הקובץ היא ברזולוציית שנייה, ושתי לחיצות מהירות על "גיבוי עכשיו" ייצרו את אותו שם — הקובץ השני דרס את הראשון בשקט. עם הגיבוי היומי זה לא נתקל; עם הכפתור זה מיידי. 194 בדיקות pytest. check-ui 13/13, check-backup 15/15, check-editor 15/15, check-offline עבר. השחזור אומת גם מול שרת חי, כולל הקובץ המוצפן. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FpBDJFA5w5AkP2YQM51JiS
Reviewer's GuideAdds a robust backup restoration script with safe/dangerous modes and end‑to‑end tests, introduces backup status and manual run APIs with UI panel, and hardens the scheduler to avoid concurrent runs, silent failures, and filename collisions. Sequence diagram for manual backup run with scheduler lockingsequenceDiagram
actor User
participant Browser
participant BackupRouter as BackupAPI
participant Scheduler
User->>Browser: click runBackupNow
Browser->>BackupRouter: POST /api/backup/run
BackupRouter->>Scheduler: run_guarded(force=True)
Scheduler->>Scheduler: is_running()
alt backup already running
Scheduler-->>BackupRouter: {ok: False, error: ALREADY_RUNNING}
BackupRouter-->>Browser: HTTP 409 CONFLICT
else backup not running
Scheduler->>Scheduler: acquire _lock
Scheduler->>Scheduler: run_once(force=True)
Scheduler->>Scheduler: write_backup()
Scheduler-->>BackupRouter: {ok: True, file, skipped}
BackupRouter-->>Browser: HTTP 200 {file, skipped}
Scheduler->>Scheduler: release _lock
end
Browser-->>User: update backup panel state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughנוספו ניהול מצב והרצה ידנית של גיבויים, נעילה נגד הרצות מקבילות, פאנל ממשק, כלי שחזור מארכיוני ZIP ומערך בדיקות למצבי שחזור ולניהול הרצות. Changesניהול גיבויים וממשק הפעלה
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator as משתמש מחובר
participant UI as index.html
participant BackupAPI as app/routers/backup.py
participant Scheduler as app/scheduler.py
participant BackupFiles as קובצי גיבוי
Operator->>UI: פתיחת פאנל הגיבוי
UI->>BackupAPI: GET /api/backup
BackupAPI->>Scheduler: קריאת מצב הריצה
Scheduler-->>BackupAPI: מצב וקובצי גיבוי
Operator->>UI: הפעלה מיידית
UI->>BackupAPI: POST /api/backup/run
BackupAPI->>Scheduler: run_guarded(force=True)
Scheduler->>BackupFiles: כתיבת גיבוי
Scheduler-->>BackupAPI: תוצאת הריצה
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary
Overall, these updates will greatly improve the system's backup functionality, improving user interaction, visibility of the backup process, and providing better error handling during backup.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_scheduler.py" line_range="217-226" />
<code_context>
+# ── נראות ומניעת ריצות מקבילות ────────────────────────────────────────
+
+
+async def test_status_reports_the_three_questions(owner, tmp_path, monkeypatch):
+ """מה שמסך ניהול צריך: פעיל? מתי אחרון? מתי הבא?"""
+ monkeypatch.setattr(scheduler, "backup_dir", lambda: tmp_path)
+ _seed(tmp_path, 3)
+
+ body = (await owner.get("/api/backup")).json()
+ assert body["enabled"] is True
+ assert body["running"] is False
+ assert body["keep"] == 30
+ assert len(body["backups"]) == 3
+ assert body["total_bytes"] > 0
+ # הבא נגזר מהאחרון ולא נשמר בנפרד, ולכן הוא נכון גם אחרי הפעלה מחדש
+ assert body["next_run_at"] is not None
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for backup status when there are no existing backups on disk.
Please also add a test for when `existing_backups()` returns an empty list. In that case, `next_run_at()` should be `None`, and the `/api/backup` response should expose `next_run_at` as `None`. You can monkeypatch `existing_backups` to return `[]` (or use an empty backup directory) and assert `body["next_run_at"] is None` so the status endpoint is covered for environments with no prior backups.
</issue_to_address>
### Comment 2
<location path="tests/test_scheduler.py" line_range="210-214" />
<code_context>
+ assert (await first)["ok"] is True
+
+
+async def test_a_failing_backup_does_not_raise_out_of_the_loop(tmp_path, monkeypatch):
+ """המתזמן קורא לכאן. חריגה שיוצאת החוצה משאירה את המערכת בלי גיבויים."""
+ monkeypatch.setattr(scheduler, "backup_dir", lambda: tmp_path)
+
+ async def boom(force=False):
+ raise RuntimeError("הדיסק מלא")
+
+ monkeypatch.setattr(scheduler, "run_once", boom)
+
+ result = await scheduler.run_guarded(force=True)
+ assert result["ok"] is False
+ assert scheduler.is_running() is False, "הנעילה לא שוחררה אחרי כישלון"
+
+ last = scheduler.last_run()
+ assert last["ok"] is False
+ assert last["error"] == "RuntimeError"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that exercises the HTTP 500 path of `/api/backup/run` on backup failure.
To also validate the API behavior, consider a router-level test: monkeypatch `scheduler.run_guarded` to return `{"ok": False, "error": "הגיבוי נכשל"}` and assert that `POST /api/backup/run` responds with HTTP 500 and the expected error payload. This will tie scheduler failures to the HTTP response and guard the API contract.
```suggestion
second = await scheduler.run_once()
assert second is None, "גיבוי שני רץ למרות שהראשון נכתב הרגע"
async def test_backup_run_returns_500_on_scheduler_failure(owner, monkeypatch):
"""וודא שכישלון בגיבוי מתורגם ל־HTTP 500 עם המטען הצפוי."""
async def failing_run_guarded(force: bool = False):
return {"ok": False, "error": "הגיבוי נכשל"}
monkeypatch.setattr(scheduler, "run_guarded", failing_run_guarded)
response = await owner.post("/api/backup/run", headers=WRITE)
assert response.status_code == 500
payload = response.json()
assert payload["detail"] == "הגיבוי נכשל"
# ── נראות ומניעת ריצות מקבילות ────────────────────────────────────────
```
</issue_to_address>
### Comment 3
<location path="tests/test_restore.py" line_range="34-38" />
<code_context>
+)
+
+# הסקריפט אינו חבילה, ולכן נטען לפי נתיב.
+_spec = importlib.util.spec_from_file_location(
+ "restore_script", Path(__file__).resolve().parent.parent / "scripts" / "restore.py"
+)
+restore_script = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(restore_script)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add focused tests for `read_archive` error paths (bad ZIP, missing/invalid encryption).
The existing round-trip and restore-mode tests exercise the happy path, but they don’t cover the failure modes mentioned by the CLI: bad ZIPs, corrupted archives, and wrong decryption passwords (`CryptoError`). Please add focused unit tests for `restore_script.read_archive` that: (1) pass non-ZIP bytes and assert the expected error output, (2) simulate a wrong passphrase by raising `CryptoError` from `do_decrypt`, and (3) cover `archive.testzip()` returning a bad member, to verify the script fails clearly and consistently in these cases.
Suggested implementation:
```python
from __future__ import annotations
import importlib.util
import io
import zipfile
import pytest
```
```python
restore_script = importlib.util.module_from_spec(_spec)
_spec.loader.exec_module(restore_script)
def _make_minimal_zip() -> io.BytesIO:
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as zf:
zf.writestr("dummy.txt", b"content")
buf.seek(0)
return buf
def test_read_archive_non_zip_bytes(capsys):
"""read_archive should fail clearly when given non-ZIP data."""
# non-zip bytes
archive_stream = io.BytesIO(b"this-is-not-a-zip")
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
# we mainly care that an error is reported; message detail can be tightened
assert captured.err.strip() != ""
def test_read_archive_wrong_passphrase_crypto_error(monkeypatch, capsys):
"""read_archive should fail clearly on CryptoError from do_decrypt (wrong passphrase)."""
archive_stream = _make_minimal_zip()
def raise_crypto_error(*args, **kwargs):
raise restore_script.CryptoError("bad passphrase")
# simulate wrong passphrase by forcing do_decrypt to raise CryptoError
monkeypatch.setattr(restore_script, "do_decrypt", raise_crypto_error)
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
assert captured.err.strip() != ""
def test_read_archive_corrupted_archive_bad_member(monkeypatch, capsys):
"""read_archive should fail clearly when archive.testzip() reports a bad member."""
archive_stream = _make_minimal_zip()
class CorruptReportingZipFile(zipfile.ZipFile):
def testzip(self):
# signal a corrupted member as zipfile.ZipFile.testzip normally does
return "bad_member.txt"
# ensure read_archive sees a ZipFile whose testzip() reports a bad member.
# if restore_script imports zipfile, patch its ZipFile; otherwise patch the global.
if hasattr(restore_script, "zipfile"):
monkeypatch.setattr(restore_script.zipfile, "ZipFile", CorruptReportingZipFile)
else:
monkeypatch.setattr(zipfile, "ZipFile", CorruptReportingZipFile)
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
assert captured.err.strip() != ""
```
These tests assume a few details that should be aligned with the actual implementation:
1. **read_archive signature**: The calls `restore_script.read_archive(archive_stream)` may need to be updated if `read_archive` expects additional parameters (e.g. `passphrase`, `output_dir`, `*, restore_mode=False`). Pass appropriate test values so that only the error path under test is triggered.
2. **Error signalling contract**: I assumed `read_archive` reports failures via `SystemExit` and writes an error message to `stderr`. If it instead raises a different exception or prints to `stdout`, adjust the `pytest.raises(...)` and `capsys.readouterr()` assertions accordingly (e.g. assert on `captured.out` or specific substrings like `"invalid zip"`, `"corrupted archive"`, or `"wrong passphrase"`).
3. **CryptoError location**: The tests reference `restore_script.CryptoError`. If `CryptoError` is imported from another module in `restore.py` (e.g. `from mycrypto import CryptoError`), and not re-exported, you can either:
- import it directly in the test module (`from mycrypto import CryptoError`) and raise that in `raise_crypto_error`, or
- expose it from `restore_script` (e.g. `CryptoError = mycrypto.CryptoError`) so the tests can use `restore_script.CryptoError`.
4. **zipfile import in restore_script**: The corrupted-member test branches depending on whether `restore_script` has a `zipfile` attribute. If `restore.py` imports `zipfile` under a different name or uses `zipfile.ZipFile` via a local alias, you should patch that alias instead so that `read_archive` uses `CorruptReportingZipFile` and its `testzip()` implementation.
5. **Minimal ZIP contents**: `_make_minimal_zip()` creates a trivial valid ZIP archive. If `read_archive` expects specific entries (e.g. a manifest or encrypted payload file), you may need to adapt the archive structure so that the function reaches the `do_decrypt` and `testzip()` logic before failing for unrelated reasons.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+217
to
+226
| async def test_status_reports_the_three_questions(owner, tmp_path, monkeypatch): | ||
| """מה שמסך ניהול צריך: פעיל? מתי אחרון? מתי הבא?""" | ||
| monkeypatch.setattr(scheduler, "backup_dir", lambda: tmp_path) | ||
| _seed(tmp_path, 3) | ||
|
|
||
| body = (await owner.get("/api/backup")).json() | ||
| assert body["enabled"] is True | ||
| assert body["running"] is False | ||
| assert body["keep"] == 30 | ||
| assert len(body["backups"]) == 3 |
There was a problem hiding this comment.
suggestion (testing): Add coverage for backup status when there are no existing backups on disk.
Please also add a test for when existing_backups() returns an empty list. In that case, next_run_at() should be None, and the /api/backup response should expose next_run_at as None. You can monkeypatch existing_backups to return [] (or use an empty backup directory) and assert body["next_run_at"] is None so the status endpoint is covered for environments with no prior backups.
Comment on lines
210
to
+214
| second = await scheduler.run_once() | ||
| assert second is None, "גיבוי שני רץ למרות שהראשון נכתב הרגע" | ||
|
|
||
|
|
||
| # ── נראות ומניעת ריצות מקבילות ──────────────────────────────────────── |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test that exercises the HTTP 500 path of /api/backup/run on backup failure.
To also validate the API behavior, consider a router-level test: monkeypatch scheduler.run_guarded to return {"ok": False, "error": "הגיבוי נכשל"} and assert that POST /api/backup/run responds with HTTP 500 and the expected error payload. This will tie scheduler failures to the HTTP response and guard the API contract.
Suggested change
| second = await scheduler.run_once() | |
| assert second is None, "גיבוי שני רץ למרות שהראשון נכתב הרגע" | |
| # ── נראות ומניעת ריצות מקבילות ──────────────────────────────────────── | |
| second = await scheduler.run_once() | |
| assert second is None, "גיבוי שני רץ למרות שהראשון נכתב הרגע" | |
| async def test_backup_run_returns_500_on_scheduler_failure(owner, monkeypatch): | |
| """וודא שכישלון בגיבוי מתורגם ל־HTTP 500 עם המטען הצפוי.""" | |
| async def failing_run_guarded(force: bool = False): | |
| return {"ok": False, "error": "הגיבוי נכשל"} | |
| monkeypatch.setattr(scheduler, "run_guarded", failing_run_guarded) | |
| response = await owner.post("/api/backup/run", headers=WRITE) | |
| assert response.status_code == 500 | |
| payload = response.json() | |
| assert payload["detail"] == "הגיבוי נכשל" | |
| # ── נראות ומניעת ריצות מקבילות ──────────────────────────────────────── |
Comment on lines
+34
to
+38
| _spec = importlib.util.spec_from_file_location( | ||
| "restore_script", Path(__file__).resolve().parent.parent / "scripts" / "restore.py" | ||
| ) | ||
| restore_script = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(restore_script) |
There was a problem hiding this comment.
suggestion (testing): Add focused tests for read_archive error paths (bad ZIP, missing/invalid encryption).
The existing round-trip and restore-mode tests exercise the happy path, but they don’t cover the failure modes mentioned by the CLI: bad ZIPs, corrupted archives, and wrong decryption passwords (CryptoError). Please add focused unit tests for restore_script.read_archive that: (1) pass non-ZIP bytes and assert the expected error output, (2) simulate a wrong passphrase by raising CryptoError from do_decrypt, and (3) cover archive.testzip() returning a bad member, to verify the script fails clearly and consistently in these cases.
Suggested implementation:
from __future__ import annotations
import importlib.util
import io
import zipfile
import pytestrestore_script = importlib.util.module_from_spec(_spec)
_spec.loader.exec_module(restore_script)
def _make_minimal_zip() -> io.BytesIO:
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as zf:
zf.writestr("dummy.txt", b"content")
buf.seek(0)
return buf
def test_read_archive_non_zip_bytes(capsys):
"""read_archive should fail clearly when given non-ZIP data."""
# non-zip bytes
archive_stream = io.BytesIO(b"this-is-not-a-zip")
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
# we mainly care that an error is reported; message detail can be tightened
assert captured.err.strip() != ""
def test_read_archive_wrong_passphrase_crypto_error(monkeypatch, capsys):
"""read_archive should fail clearly on CryptoError from do_decrypt (wrong passphrase)."""
archive_stream = _make_minimal_zip()
def raise_crypto_error(*args, **kwargs):
raise restore_script.CryptoError("bad passphrase")
# simulate wrong passphrase by forcing do_decrypt to raise CryptoError
monkeypatch.setattr(restore_script, "do_decrypt", raise_crypto_error)
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
assert captured.err.strip() != ""
def test_read_archive_corrupted_archive_bad_member(monkeypatch, capsys):
"""read_archive should fail clearly when archive.testzip() reports a bad member."""
archive_stream = _make_minimal_zip()
class CorruptReportingZipFile(zipfile.ZipFile):
def testzip(self):
# signal a corrupted member as zipfile.ZipFile.testzip normally does
return "bad_member.txt"
# ensure read_archive sees a ZipFile whose testzip() reports a bad member.
# if restore_script imports zipfile, patch its ZipFile; otherwise patch the global.
if hasattr(restore_script, "zipfile"):
monkeypatch.setattr(restore_script.zipfile, "ZipFile", CorruptReportingZipFile)
else:
monkeypatch.setattr(zipfile, "ZipFile", CorruptReportingZipFile)
with pytest.raises(SystemExit):
# adjust arguments to match restore_script.read_archive signature
restore_script.read_archive(archive_stream)
captured = capsys.readouterr()
assert captured.err.strip() != ""These tests assume a few details that should be aligned with the actual implementation:
- read_archive signature: The calls
restore_script.read_archive(archive_stream)may need to be updated ifread_archiveexpects additional parameters (e.g.passphrase,output_dir,*, restore_mode=False). Pass appropriate test values so that only the error path under test is triggered. - Error signalling contract: I assumed
read_archivereports failures viaSystemExitand writes an error message tostderr. If it instead raises a different exception or prints tostdout, adjust thepytest.raises(...)andcapsys.readouterr()assertions accordingly (e.g. assert oncaptured.outor specific substrings like"invalid zip","corrupted archive", or"wrong passphrase"). - CryptoError location: The tests reference
restore_script.CryptoError. IfCryptoErroris imported from another module inrestore.py(e.g.from mycrypto import CryptoError), and not re-exported, you can either:- import it directly in the test module (
from mycrypto import CryptoError) and raise that inraise_crypto_error, or - expose it from
restore_script(e.g.CryptoError = mycrypto.CryptoError) so the tests can userestore_script.CryptoError.
- import it directly in the test module (
- zipfile import in restore_script: The corrupted-member test branches depending on whether
restore_scripthas azipfileattribute. Ifrestore.pyimportszipfileunder a different name or useszipfile.ZipFilevia a local alias, you should patch that alias instead so thatread_archiveusesCorruptReportingZipFileand itstestzip()implementation. - Minimal ZIP contents:
_make_minimal_zip()creates a trivial valid ZIP archive. Ifread_archiveexpects specific entries (e.g. a manifest or encrypted payload file), you may need to adapt the archive structure so that the function reaches thedo_decryptandtestzip()logic before failing for unrelated reasons.
PR Summary by Qodoשחזור מגיבוי + נראות והפעלה ידנית למנגנון הגיבוי
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
scripts/restore.py (3)
302-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueהקדימו את בדיקת פרטי ההתחברות לפני האישור.
בדיקת
--passwordנמצאת אחרי שאלת האישור. המשתמש קורא את אזהרתreplace, מאשר, ורק אז מקבל "חסרים --email/--password". העבירו את הבדיקה לפני שורה 302.Claude Code כתב כאן זרימה נעימה לקריאה, וברירת המחדל השלילית ב-
[y/N]היא בחירה נכונה. זו רק שאלה של סדר.♻️ סדר מוצע
+ if not args.email or not args.password: + print("חסרים --email/--password או ADMIN_EMAIL/ADMIN_PASSWORD") + return 1 + if not args.yes: answer = input("להמשיך? [y/N] ").strip().lower() if answer not in ("y", "yes"): print("בוטל.") return 1 - - if not args.email or not args.password: - print("חסרים --email/--password או ADMIN_EMAIL/ADMIN_PASSWORD") - return 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/restore.py` around lines 302 - 311, Move the `args.email`/`args.password` validation block before the `if not args.yes` confirmation flow, so missing credentials are reported before prompting the user. Preserve the existing default-negative `[y/N]` behavior and cancellation return path.
60-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winהוסיפו אבחון לתשובת הפניה (redirect) בכניסה.
הלקוח מוגדר עם
follow_redirects=False. אם--urlמפנה מחדש (למשלhttp://אלhttps://, או כתובת עם סיומת שונה), הכניסה מחזירה 301/307. ההודעה הנוכחית מדווחת "הכניסה נכשלה" ומכוונת את המשתמש לבדוק מייל וסיסמה, וזה מסתיר את הסיבה האמיתית. הוסיפו הודעה נפרדת למקרה של הפניה.♻️ שיפור מוצע להודעת השגיאה
if response.status_code != 200: + if response.is_redirect: + target = response.headers.get("location", "") + sys.exit( + f"הכתובת מפנה מחדש אל {target}. הריצו שוב עם הכתובת הסופית ב---url." + ) sys.exit(f"הכניסה נכשלה ({response.status_code}). בדקו כתובת, מייל וסיסמה.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/restore.py` around lines 60 - 68, Update login in the response-status handling to detect redirect status codes and emit a separate message identifying the redirect and its target, instead of treating them as credential failures; preserve the existing generic failure message for non-redirect statuses.
109-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winהאחידו את הטיפול בארכיון פגום.
הסקריפט מסיים כל תקלה אחרת בהודעה בעברית, אבל
json.loadsעלmanifest.jsonוגםdecode("utf-8")על המסמכים מעלים חריגה גולמית. משתמש שמקבל ארכיון חתוך רואה traceback במקום הסבר. עטפו את קריאת המניפסט ואת פענוח התוכן בהודעת שגיאה מפורשת.הערה נוספת:
archive.testzip()מפרק את כל חברי הארכיון לצורך בדיקת CRC, והמסמכים נקראים שוב אחר כך. עבור גיבוי גדול זו עבודה כפולה. אם זה חשם לכם — אפשר להסתפק בבדיקת ה-CRC הנעשית ממילא בכלarchive.read.♻️ תיקון מוצע לקריאת המניפסט
manifest = {} if "manifest.json" in names: - manifest = json.loads(archive.read("manifest.json")) + try: + manifest = json.loads(archive.read("manifest.json")) + except (json.JSONDecodeError, UnicodeDecodeError): + sys.exit("manifest.json בארכיון אינו JSON תקין.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/restore.py` around lines 109 - 115, אחדו את טיפול השגיאות בזרימת קריאת archive: עטפו את json.loads בעת טעינת manifest.json ואת decode("utf-8") בעת קריאת המסמכים, והציגו הודעת שגיאה מפורשת בעברית במקום traceback. הסירו את בדיקת archive.testzip() המוקדמת, כך שבדיקות ה-CRC יתבצעו במסגרת archive.read הקיימת ללא קריאה כפולה.tests/test_restore.py (1)
42-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winתקנו את ה-docstring, והשתמשו ב-
callsאו הסירו אותו.שני פרטים בקטע הזה:
- שורה 44 קובעת "הסקריפט סינכרוני; הבדיקות אסינכרוניות". זה לא נכון: כל המתודות ב-
Clientשב-scripts/restore.pyהןasync def, וה-docstring שם (שורות 48-50) מסביר במפורש שהוא אסינכרוני כדי שאותהrestoreתרוץ גם מהבדיקות. ההסבר הנוכחי מטעה את מי שיקרא את המחלקה הזאת בפעם הראשונה. הסיבה האמיתית לקיומה היא ניתוב אל ה-ASGI client והוספת כותרותWRITE.self.callsנאסף בכל מתודה ואף בדיקה לא קוראת אותו. או שתסירו אותו, או שתשתמשו בו כדי לאמת את המצבים — למשל שב-skipעל פרויקט קיים אין בכללPUT.♻️ תיקון מוצע ל-docstring
- """מתרגם את ממשק ה-Client של הסקריפט ל-httpx האסינכרוני של הבדיקות. - - הסקריפט סינכרוני; הבדיקות אסינכרוניות. במקום להרים שרת, כל קריאה - מנותבת לאותו client שכל שאר הבדיקות משתמשות בו — כלומר מה שנבדק - הוא הלוגיקה של השחזור, ולא httpx. - """ + """מממש את ממשק ה-Client של הסקריפט מול ה-ASGI client של הבדיקות. + + הסקריפט אסינכרוני, ולכן אותה `restore` רצה כאן בלי גרסה שנייה של + הלוגיקה. במקום להרים שרת, כל קריאה מנותבת לאותו client שכל שאר + הבדיקות משתמשות בו, עם כותרות WRITE — כלומר מה שנבדק הוא הלוגיקה + של השחזור, ולא httpx. + """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_restore.py` around lines 42 - 52, Update the adapter class docstring to accurately describe routing restore requests through the ASGI client and adding WRITE headers, without claiming the script is synchronous; then either remove the unused self.calls tracking from the adapter methods or add assertions that use it, including verifying skip mode for an existing project performs no PUT.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/restore.py`:
- Around line 256-260: Update scripts/restore.py lines 256-260 in the
link-restore loop to fetch existing links through GET /links, filter by URL, and
POST only links that are not already present. Update tests/test_restore.py lines
267-281 to add a project link, run the second restore in upsert mode, and assert
the project still has exactly one link.
- Around line 118-131: Update the archive-entry loop around name.partition("/")
to skip any record whose leaf still contains "/", before creating or updating a
project entry. This must ignore nested paths such as alpha/draft/install.md and
helper directories such as __MACOSX/ while preserving processing of flat
links.json and Markdown document entries.
- Around line 269-270: Remove the --password argument from the argument parser
and obtain the admin password only from ADMIN_PASSWORD or getpass before the
login flow. Update the validation and subsequent authentication logic to use the
resolved password variable, while preserving the existing --email/ADMIN_EMAIL
handling and --yes behavior.
- Around line 180-215: בדקו את תגובת DELETE במסלול replace לפני לקבוע
project_exists = False ולהמשיך ליצירה; عند כשל, הוסיפו אזהרה מתאימה ל־warnings
והמשיכו לטפל בשחזור לפי הזרימה הקיימת. בדקו גם את תגובת PATCH במסלול שבו הפרויקט
קיים, והוסיפו אזהרה ל־warnings כאשר עדכון description או visibility נכשל, תוך
שמירת actual_slug והמשך עיבוד הפרויקט.
In `@tests/test_restore.py`:
- Around line 95-115: Replace the duplicated archive parsing in the test helper
_read with the production read_archive function from scripts/restore.py, writing
the raw bytes to a temporary path and calling read_archive on it. Update each
test to use the read fixture as read(raw) instead of _read(raw), add the
required itertools import, and remove zipfile/json imports only if unused
afterward.
---
Nitpick comments:
In `@scripts/restore.py`:
- Around line 302-311: Move the `args.email`/`args.password` validation block
before the `if not args.yes` confirmation flow, so missing credentials are
reported before prompting the user. Preserve the existing default-negative
`[y/N]` behavior and cancellation return path.
- Around line 60-68: Update login in the response-status handling to detect
redirect status codes and emit a separate message identifying the redirect and
its target, instead of treating them as credential failures; preserve the
existing generic failure message for non-redirect statuses.
- Around line 109-115: אחדו את טיפול השגיאות בזרימת קריאת archive: עטפו את
json.loads בעת טעינת manifest.json ואת decode("utf-8") בעת קריאת המסמכים, והציגו
הודעת שגיאה מפורשת בעברית במקום traceback. הסירו את בדיקת archive.testzip()
המוקדמת, כך שבדיקות ה-CRC יתבצעו במסגרת archive.read הקיימת ללא קריאה כפולה.
In `@tests/test_restore.py`:
- Around line 42-52: Update the adapter class docstring to accurately describe
routing restore requests through the ASGI client and adding WRITE headers,
without claiming the script is synchronous; then either remove the unused
self.calls tracking from the adapter methods or add assertions that use it,
including verifying skip mode for an existing project performs no PUT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4117c434-88e3-4853-9398-5c4cc105ca8b
📒 Files selected for processing (7)
ROADMAP.mdapp/routers/backup.pyapp/scheduler.pyindex.htmlscripts/restore.pytests/test_restore.pytests/test_scheduler.py
הממצא המהותי: השחזור דיווח הצלחה על מסמכים שלא נכתבו. השרת מחזיר 200 עם applied=false כשהוא דוחה כתיבה שאינה מתקדמת מאותו עורך, והסקריפט השתמש ב-editor_id קבוע ובדק סטטוס בלבד. כלומר שחזור שני של אותו מסמך נדחה בשקט ונספר כהצלחה. שוחזר מול שרת חי לפני התיקון. עכשיו מזהה עורך ייחודי לכל ריצה, ובדיקת applied — כתיבה שנדחתה נספרת כאזהרה ולא כהצלחה. עוד בשחזור: - קישורים שוכפלו בכל הרצה חוזרת ב-upsert. אין אילוץ ייחודיות שימנע את זה, ולכן הקיימים נקראים ומסוננים לפי URL. גם position נשמר עכשיו — בלעדיו הסדר שהכותב קבע אבד. - upsert לא יישר את שם הפרויקט, ולכן שם שהשתנה אחרי הגיבוי שרד שחזור שאמור להחזיר את מה שגובה. - תגובות DELETE ו-PATCH נבדקות. מחיקה שנכשלה לפני replace הובילה ליצירה שנכשלת ב-409, בזמן שהפלט מדווח על שחזור. - --password הוסר. ארגומנטים נראים ב-ps ונשמרים בהיסטוריית ה-shell, והסיסמה נקראת מ-ADMIN_PASSWORD או מ-getpass. - נתיבים מקוננים ותיקיות __MACOSX מדולגים. הארכיון שלנו שטוח, אבל הקובץ מגיע מבחוץ. - שגיאות קריאה מוצגות בעברית במקום traceback. ב-API: backup_status עשה שני מעברי stat לכל קובץ בלי טיפול בשגיאה, וקובץ שנמחק בין המעברים היה מחזיר 500 מהמסך שאמור לדווח על מצב הגיבוי. עכשיו מעבר אחד עם דילוג על מה שנעלם. stamp_of הפך לציבורי במקום שימוש חיצוני בפרטי. על הנעילה — הביקורת ביקשה try-acquire במקום בדיקה ואז תפיסה. ניסיתי, ו-wait_for(acquire(), timeout=0) מבטל את המשימה לפני שהיא רצה: הוא נכשל תמיד, גם כשהנעילה פנויה, ושום גיבוי לא היה רץ יותר. הבדיקה שיורה 20 קריאות במקביל תפסה את זה בתלייה. ל-asyncio.Lock אין try-acquire, והצירוף הקיים בטוח כי acquire אינו משתהה כשהנעילה פנויה — נשאר כפי שהיה, עם נימוק כתוב ובדיקה שמוכיחה אותו במקום להסתמך על הנימוק. בבדיקות: עוזר הקריאה בטסטים היה עותק של לוגיקת הפרסור, והוחלף בקריאה ל-read_archive עצמה. נוספו מסלולי הכשל שביקשה הביקורת — קובץ שאינו ZIP, סיסמה שגויה, CRC שבור, נתיבים מקוננים — וכן מצב בלי גיבויים כלל, כשל שמתורגם ל-500, וקובץ שנעלם באמצע הקריאה. 209 בדיקות pytest. check-ui, check-editor, check-backup, check-offline — כולם עברו. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FpBDJFA5w5AkP2YQM51JiS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
הפער שנסגר
היה גיבוי ולא הייתה דרך לשחזר ממנו. הארכיון נפתח יפה וקריא לגמרי — אבל כדי להחזיר את התוכן למערכת היה צריך ליצור כל פרויקט מחדש ולהדביק כל מסמך ביד. ארכיון שאפשר לקרוא אינו גיבוי.
בנוסף, המנגנון רץ בלי שאף אחד רואה אותו: מתי גובה לאחרונה ומתי הבא היו שאלות שהתשובה להן ישבה ב-log של Render בלבד.
שני באגים שקטים שהבדיקה מצאה
הבדיקה שסוגרת את המעגל — יצירה → גיבוי → מחיקת הכול → שחזור → השוואה — מצאה אותם ברגע שנכתבה:
הפרויקט נוצר מחדש עם
slugשנגזר מהשם, והמסמכים עםslugשנגזר מהכותרת, במקום אלה שנשמרו בגיבוי. השחזור היה "מצליח" והתוכן חזר — תחת כתובות אחרות. כל קישור שנשלח למישהו היה נשבר.ה-API כבר קיבל
slugמפורש ביצירה; הסקריפט פשוט לא שלח אותו.אומת מקצה לקצה מול שרת חי:
slug,visibility, שם, קישורים ותוכן חוזרים זהים בדיוק.scripts/restore.pyשלושה מצבי כתיבה, מהבטוח למסוכן:
skip(ברירת מחדל)upsertreplaceלפני כל כתיבה מוצג בדיוק מה עומד לקרות, והאישור נשאל עם ברירת מחדל
N— Enter בטעות לא משחזר כלום. יש--dry-run, ויש--decryptלקובץ המוצפן שמגיע מטלגרם. הסיסמה נקראת ממשתנה סביבה או מ-stdin, לעולם לא כארגומנט — ארגומנטים נכנסים להיסטוריית ה-shell ונראים ב-ps.הסקריפט הפך לאסינכרוני כדי שאותה פונקציית
restoreתרוץ גם מה-CLI וגם מהבדיקות מול ה-ASGI client. הגרסה הראשונה של הבדיקה המירה את הקוד הסינכרוני לאסינכרוני עםexecעל המקור — פתרון שנשבר בשקט בכל שינוי מבנה.נראות
GET /api/backupמחזיר את שלוש השאלות שחשובות — פעיל, מתי אחרון, מתי הבא — ואת רשימת הקבצים.POST /api/backup/runמריץ ידנית. בממשק נוסף פאנל שמציג אותן ולחצן "גיבוי עכשיו".הנעילה נבדקת עם
is_running()לפני התפיסה ולא במקומה:async withלבדו היה גורם לבקשה שנייה להמתין לסיום ואז להריץ גיבוי נוסף. עכשיו היא חוזרת מיד עם 409 ולא 500 — תפוס אינו שבור, ולמשתמש מגיע לדעת מה מהשניים.באג נוסף שהתגלה בדרך
החותמת בשם הקובץ היא ברזולוציית שנייה. שתי לחיצות מהירות על "גיבוי עכשיו" ייצרו את אותו שם, והקובץ השני דרס את הראשון בשקט. עם הגיבוי היומי זה לא נתקל; עם הכפתור זה מיידי.
ולידציה
check-ui13/13 ·check-backup15/15 ·check-editor15/15 ·check-offlineעבר--decrypt, עם סיסמה שגויה, ועם הנכונהapp/ועל הסקריפטGenerated by Claude Code
Summary by Sourcery
Add a full backup visibility and manual run flow, and implement a tested restore-from-backup script that preserves project and document identity.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: